perf: skip archived repos without fetching them - #1065
Open
rafaelleonardocruz wants to merge 2 commits into
Open
perf: skip archived repos without fetching them#1065rafaelleonardocruz wants to merge 2 commits into
rafaelleonardocruz wants to merge 2 commits into
Conversation
`updateRepos` already skips archived repos (github-community-projects#991), but only after `archivePlugin.getState()` has spent a `repos.get` on each one. That call is avoidable: `GET /installation/repositories`, which `eachRepositoryRepos` already paginates, reports `archived` in its payload. Thread that flag through `checkAndProcessRepo` into `updateRepos` and skip before issuing any request. The saving is one API call per archived repo per full sync. In the organization where I found this, 2228 of 3013 repos (74%) are archived, so the majority of a full sync's rate-limit budget was spent fetching repos only to skip them — and on an installation the rate limit, not concurrency, is what bounds how long a full sync takes. The skip is conditional on the desired state, so an explicit `archived: false` in config is still processed: that is a request to unarchive. `getDesiredArchiveState()` reads config only and issues no request. Callers that do not know the archived state (single-repo webhook syncs) pass `undefined` and keep the existing behaviour, falling through to the `isArchived` check from github-community-projects#991. Four tests added: skip with no fetch when the caller reports archived, still fetch when config asks to unarchive, still fetch when the caller does not report the state, and the flag being threaded from the listing into `updateRepos`. Co-Authored-By: Claude <noreply@anthropic.com> AI-Assisted: yes AI-Tool: claude-code Co-Authored-By: claude-code <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR optimizes full-sync performance by using the archived flag already present in the GET /installation/repositories listing payload to skip processing archived repositories before making per-repo API calls.
Changes:
- Thread
repository.archivedfromeachRepositoryRepos→checkAndProcessRepo→updateRepos. - Add an early guard in
updateReposto skip archived repos without callingrepos.get, unless config explicitly requests unarchiving. - Add unit tests asserting when
repos.getis (and is not) called and thatarchivedis correctly threaded through.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| lib/settings.js | Pass archived from repo listing into updateRepos and add an early skip to avoid unnecessary API calls for archived repos. |
| test/unit/lib/settings.test.js | Add unit tests covering early-skip behavior and verifying archived is threaded into updateRepos. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Two review findings, both valid.
Copilot: threading `archived` on the repo object leaks it into plugins. The
Repository plugin does `Object.assign({}, settings, repo)` (plugins/repository.js:48)
with `repo` last, and later `repos.update(this.settings)` (:215) — so an
`archived` key on the ref lands in the update payload. In the unarchive flow that
is destructive: the repo is unarchived, then the Repository plugin PATCHes
`archived: true` back and re-archives it. `repos.get(this.repo)` (:67) would also
receive a stray parameter. `archived` is now a separate argument to updateRepos
and never touches the ref; plugins keep receiving a bare { owner, repo }.
Second finding: the guard only covered the `repoConfig` branch. Without a
repoConfig — a labels-only configuration, for instance — the else branch ran every
child plugin with no archive check, so an archived repo still received forbidden
writes. Added the same guard there, conditional on `archived !== false` so it
costs nothing on the full-sync path: `false` from the listing needs no request,
and `true` already returned earlier unless an unarchive was requested. Only a
caller that does not know the state (single-repo webhook sync) pays one repos.get.
Also dropped the duplicate Archive instantiation — the hoisted one is reused.
Tests 141 -> 143. Two new: the no-repoConfig path skips child plugins for an
archived repo, and `archived` does not appear on the ref passed to updateRepos.
The threading test now asserts the argument position rather than a merged object.
The labels stub is deliberately complete (endpoint.merge + paginate) so the
no-write assertion fails loudly instead of passing because the plugin crashed.
Co-Authored-By: Claude <noreply@anthropic.com>
AI-Assisted: yes
AI-Tool: claude-code
Co-Authored-By: claude-code <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
updateReposalready skips archived repos — #991 added:But that check runs after
archivePlugin.getState(), which callsrepos.get. So every archived repo still costs one API call per full sync, purely to learn something the caller already knew.eachRepositoryRepospaginatesGET /installation/repositories, and that response already carriesarchivedfor every repo. Today onlyowner.loginandnameare read from it.Why it matters
On an installation, the rate limit — not fan-out concurrency — is what bounds how long a full sync takes. Wasted calls translate directly into wall-clock time.
In the organization where I hit this, 2228 of 3013 repos (74%) are archived, so the clear majority of the sync's request budget went to fetching repos in order to skip them. A secondary effect: each archived repo that isn't skipped early still emits 403s from the repository and labels plugins, which buries genuine errors in the log — I had to reconstruct the ratio arithmetically to tell signal from noise while debugging an unrelated problem.
Change
Thread
archivedfrom the listing throughcheckAndProcessRepointoupdateRepos, and skip before issuing any request.Net effect: one fewer API call per archived repo per full sync.
Correctness notes
archived: falsein config is still processed — that is a request to unarchive.getDesiredArchiveState()reads config only and issues no request, so the guard stays free.Settings.syncandsyncSelectedReposbuildrepowithout anarchivedfield. Those passundefined, the new guard does not fire, and they fall through to the existingisArchivedcheck from Bug/archived repo #991 — same behaviour as today.repoConfigis fully resolved before the desired state is read.Tests
Four added to
test/unit/lib/settings.test.js, asserting on whetherrepos.getwas called:archivedis threaded from the repository listing intoupdateReposnpx jest --roots=lib --roots=test/unit→ 141 passing, 0 failing.eslintclean on both changed files (the 69 pre-existingsemi/quotesissues elsewhere in the test file are untouched).One note on the test setup, in case it helps future tests: the file's
createSettingshelper passesmockSubOrg, which setssubOrgConfigMapand makesupdateReposreturn early for any repo outside that suborg. These tests constructSettingswithout a suborg for that reason.